-
Notifications
You must be signed in to change notification settings - Fork 686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change CALL_ONCE_INITIALIZATION implementation to use static initialization #4818
Conversation
…zation Apply LLVM upstream patch https://reviews.llvm.org/D19271. The previous implementation deadlocks when used concurrently from many threads at once.
There were a lot of challenges back in 2016 related to libstdc++ having a broken Now that |
@llvm-beanz thanks for checking in; I may not have included all the context from #4792 in this patch but the gist of it is that my backporting of
Perhaps I did not pick all the patches needed to finalize the migration, but it seems simple enough to not suffer from any deadlocks already. Suggestions/help welcome! |
Yes I find C++ and everything terrifying and weird all around, which is why I've switched over to Rust 😉 |
I don't think we should use |
Does something like this not work? #define CALL_ONCE_INITIALIZATION(function) \
std::once_flag flag; \
std::call_once(flag, [&Registry]() { function(Registry); }); |
https://en.cppreference.com/w/cpp/thread/once_flag I think the std::once_flag object would need to be static. |
Yes, you are correct. |
I don't know if any scope related issues will pop up, but just in case:
|
There shouldn't be any scoping issues. The existing implementations of the macros don't have problems and these are only used as macros inside macros... |
Hmm, i'm getting build errors saying std::call_once is not a member of std. I included mutex at the top as well. |
Ah, never mind. I had to write it like this to get it to compile:
|
First up, while looking at this patch on a 2-week-old DXC at b66e05be9458677faf5dcf4eeda76dad6b702738 (somewhere between then and now a SPIR-V codegen bug snuck in, that I have not yet had the opportunity to bisect and report), it now also locked up right at:
Something really fishy is going on. I'll get to testing the above suggestion soon, but all seems futile now (or my machine is just broken?). |
That's fun... I wish I could say I'm surprised, but I spent enough time debugging LLVM's call_once implementation, so I'm not at all surprised. The C++ standard dictates that static initialization is thread-safe, but I'm not actually surprised that this might not work as expected. One of the things on my to-do list is to try building DXC with thread sanitizer enabled to see if we can identify issues like this. |
It is surprising, because the patch did solve the lockups on our CI completely... To be able to reproduce it, repeatedly on my own machine without much effort is incredibly strange. Let me know when you get thread sanitizer to work, I'll gladly run it in hopes of getting to the bottom of this. |
Adding |
Thanks! Trying with the following full command:
Note that per #4792 (comment) |
|
Weird... The only difference in my build is I also pass |
Hmm, that doesn't seem to make a difference. I even tried hacking in a (probably wrong) |
That's unexpected... What is your host OS and CPU architecture? Can you attach a debugger and figure out what is mapped in that region? TSan needs to identify the mapped memory regions and it looks like your process has something mapped TSan isn't expecting. That might be a build configuration issue with your toolchain, or it might be a bug in TSan, or something completely different... |
As said I may have added that wrongly. Now, adding: if(LLVM_USE_SANITIZER)
target_link_libraries(dxcompiler PUBLIC tsan)
endif () right after |
Yea... none of that should be needed. Clang should add the appropriate linker flags based on the sanitizer usage, and I would expect libtsan.so is probably not the correct runtime library. I would instead have expected a static linkage against libclang_rt.tsan-x86_64.a and libclang_rt.tsan-cxx-x86_64.a (unless you're not linux-x86_64). What is your host OS and CPU? |
Right, yes, static makes more sense for this. No idea why it isn't happening. Host OS is Arch Linux (syncing a few times a week), CPU is a 3970x. Will play around with this on a different albeit similar system and see if/where the libraries are, and why they're not linked against, in turn leaving |
Running into the same @llvm-beanz just making sure: are you loading $ nm -D DirectXShaderCompiler/build/bin/dxc | rg tsan_init
00000000000c8060 T __tsan_init
$ nm -D DirectXShaderCompiler/build/lib/libdxcompiler.so | rg tsan_init
U __tsan_init In turn making it seem like this is working via the |
Yea. In order for tsan to work properly all parts of your binary need to be built with it enabled, and the runtime initialization stub gets statically linked into the main executable. |
@llvm-beanz thanks for the suggestion, at least it's linking now with Quite a lot of noise pointing into the EDIT: Note again that this is a build based on top of 24ca1f4, haven't yet tested if our SPIR-V issues have been resolved on EDIT2: Deleted the output, it was from a build without tsan in dxc 🙃 Unsure if it is useful/possible to catch the hang this way, and if it has an effect on whatever tsan would emit. |
Hmm, now that I'm running with EDIT: this is on top of 24ca1f4 with the $ rg SUMMARY tsan | sort -u
...
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/include/llvm/Support/ManagedStatic.h:66:17 in llvm::ManagedStatic<llvm::sys::SmartMutex<true> >::operator*()
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/lib/DxcSupport/Unicode.cpp:114:3 in WideCharToMultiByte(unsigned int, unsigned int, wchar_t const*, int, char*, int, char const*, bool*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/lib/Support/Mutex.cpp:86:58 in llvm::sys::MutexImpl::acquire()
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/lib/Support/Mutex.cpp:89:19 in llvm::sys::MutexImpl::acquire()
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/lib/Support/raw_ostream.cpp:691:7 in llvm::raw_fd_ostream::preferred_buffer_size() const
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/lib/Support/Unix/Signals.inc:122:7 in RegisterHandlers()
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:10120:3 in clang::Sema::DeclareImplicitCopyAssignment(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:10945:3 in clang::Sema::DeclareImplicitCopyConstructor(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:6485:5 in clang::Sema::AddImplicitlyDeclaredMembersToClass(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:6488:5 in clang::Sema::AddImplicitlyDeclaredMembersToClass(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:6505:5 in clang::Sema::AddImplicitlyDeclaredMembersToClass(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:6526:5 in clang::Sema::AddImplicitlyDeclaredMembersToClass(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:8972:3 in clang::Sema::DeclareImplicitDefaultConstructor(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaDeclCXX.cpp:9440:3 in clang::Sema::DeclareImplicitDestructor(clang::CXXRecordDecl*)
SUMMARY: ThreadSanitizer: data race /newdata/packages/directx-shader-compiler-git/src/DirectXShaderCompiler/tools/clang/lib/Sema/SemaHLSL.cpp in ConvertDimensions(ArTypeInfo, ArTypeInfo, clang::ImplicitConversionKind&, TYPE_CONVERSION_REMARKS&)
SUMMARY: ThreadSanitizer: heap-use-after-free /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/12.2.0/../../../../include/c++/12.2.0/bits/char_traits.h:431:33 in std::char_traits<char>::copy(char*, char const*, unsigned long) |
This PR seems to have been inactive for quite a while now, so I'm going to close it. Feel free to reactive if you want to rebase it and continue working on a solution in DXC. |
We're still building wit this patch to prevent the aforementioned issues. I don't have permissions to reopen (what you call "reactive"..ate?), and rebasing breaks the reopening feature because GitHub things you want to use the branch for something else. |
Fixes #4792, see that issue for more context.
Apply LLVM upstream patch https://reviews.llvm.org/D19271.
The previous implementation deadlocks when used concurrently from many threads at once.